Flip incompatible_disable_autoloads_in_main_repo #28
Conversation
The flag flip forces loads in main repository when using Bazel@HEAD. Flipping it now, so that we can disable autoloads by release of Bazel 9. PiperOrigin-RevId: 745526309 Change-Id: I7204470c08a66a15b28bc0b0c48f41d69e1694c8
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @arvi18, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
I've flipped the incompatible_disable_autoloads_in_main_repo flag to true as a preparatory step for Bazel 9, which will disable implicit rule loading. This change necessitates explicit load statements for all rules in BUILD files, and I've updated numerous examples and test setups to reflect this new requirement. The goal is to ensure that all necessary rules are explicitly declared, preventing build failures once autoloads are fully deprecated.
Highlights
- Incompatible Flag Flip: The core change in this pull request is the flipping of the
incompatible_disable_autoloads_in_main_repoflag's default value fromfalsetotruewithinBuildLanguageOptions.java. This is a significant step towards Bazel 9, where autoloads will be disabled, requiring explicitloadstatements for all rules. - Widespread Explicit Rule Loading: As a direct consequence of disabling autoloads, numerous
BUILDfiles across various examples and source directories have been updated. This includes adding explicitloadstatements for commonly used rules such aspy_binary,py_library,py_test,sh_binary,sh_library,sh_test,java_binary,java_library,java_import,java_plugin,cc_library,cc_binary,cc_import,cc_test,proto_library, andjava_proto_librarywhere they were previously implicitly available. - Comprehensive Test Infrastructure Updates: The pull request also includes extensive updates to Python and shell test infrastructure. This involves modifying test scripts to ensure that
bazel_depentries for rulesets likerules_python,rules_shell,rules_java, andrules_ccare correctly added toMODULE.bazelfiles, and thatloadstatements are explicitly present inBUILDfiles generated within tests. These changes are crucial for maintaining test functionality under the new autoloading behavior.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request flips the incompatible_disable_autoloads_in_main_repo flag, which is a significant step towards modernizing Bazel's build language by requiring explicit load() statements. The changes are extensive and mostly correct. My review includes suggestions to improve maintainability by combining load statements, and fixes for a few test setups where dependencies were missing or code was malformed. Overall, this is a solid contribution.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (4)
src/test/shell/testenv.sh (1)
476-489: Ensure explicitloadforjava_importeven whenjunit4target already existsWhen
third_party/BUILDalready declaresname = "junit4"(from a legacy setup), the current guard skips appending the block—and thus never adds the requiredload("@rules_java//java:java_import.bzl", "java_import"). With autoloads disabled, this will break the BUILD file.Suggested patch:
function setup_javatest_support() { setup_javatest_common - grep -q 'name = "junit4"' third_party/BUILD \ - || cat <<EOF >>third_party/BUILD - load("@rules_java//java:java_import.bzl", "java_import") - - java_import( - name = "junit4", - jars = [ - "junit.jar", - "hamcrest.jar", - ], - ) - EOF + # Append junit4 target + load if missing entirely + grep -q 'name = "junit4"' third_party/BUILD \ + || cat <<EOF >>third_party/BUILD +load("@rules_java//java:java_import.bzl", "java_import") + +java_import( + name = "junit4", + jars = [ + "junit.jar", + "hamcrest.jar", + ], +) +EOF + + # If junit4 exists but the load is not present, prepend it + if grep -q 'name = "junit4"' third_party/BUILD \ + && ! grep -q '@rules_java//java:java_import.bzl' third_party/BUILD; then + tmp="$(mktemp)" + { + echo 'load("@rules_java//java:java_import.bzl", "java_import")' + cat third_party/BUILD + } > "$tmp" && mv "$tmp" third_party/BUILD + fi }
- This guarantees the
loadstatement is present regardless of howjunit4was first added.- No other files are affected.
src/test/shell/bazel/bazel_proto_library_test.sh (2)
54-61: String-comparison bug –-eqis for integers, not strings
if [ "${include_macro}" -eq "" ]; thenwill raise
“integer expression expected” whenever${include_macro}is non-empty.
Use string operators instead:-if [ "${include_macro}" -eq "" ]; then +if [ -z "${include_macro}" ]; then
282-286: Broken quote in generated BUILD file
export_files(["proto_library_macro.bzl])is missing the closing
quotation mark and will yield a syntax error when the file is parsed.-export_files(["proto_library_macro.bzl]) +export_files(["proto_library_macro.bzl"])src/test/shell/bazel/bazel_coverage_cc_test_gcc.sh (1)
536-538: Unquoted argument – will split and break on spaces
add_rules_cc MODULE.bazelis missing quotes; the shell passes two
separate arguments when the path contains spaces, breaking the helper.-add_rules_cc MODULE.bazel +add_rules_cc "MODULE.bazel"
♻️ Duplicate comments (1)
src/test/shell/bazel/remote/remote_build_event_uploader_test.sh (1)
456-456: Same initialization suggestion applies here.See the earlier comment about moving
add_rules_shell "MODULE.bazel"intoset_up().
🧹 Nitpick comments (15)
src/test/shell/bazel/bazel_random_characters_test.sh (1)
52-52: Quote the MODULE.bazel argument for robustness.While unquoted works here, quoting avoids surprises and matches the rest of the repo.
- add_rules_java MODULE.bazel + add_rules_java "MODULE.bazel"src/test/shell/bazel/remote/remote_build_event_uploader_test.sh (1)
428-428: Initialize rules in set_up() once to avoid duplication.Calling
add_rules_shellper test works if it's idempotent, but moving it toset_up()reduces repetition and the risk of duplicating MODULE content.Example adjustment (outside current hunk):
function set_up() { add_rules_shell "MODULE.bazel" start_worker }Then remove
add_rules_shell "MODULE.bazel"from individual tests.src/test/shell/bazel/bazel_rules_java_override_test.sh (2)
77-78: Declare rules_java module before use — goodadd_rules_java "MODULE.bazel" before generating the BUILD is the right ordering.
If multiple tests in this file need rules_java, consider moving add_rules_java into a file-level set_up() to avoid duplication.
91-92: Redundant per-test add_rules_javaThis mirrors the earlier test. Works as-is. Optionally factor into set_up() for dedup.
src/test/shell/bazel/bazel_sandboxing_test.sh (1)
332-336: C++ rules declared and loaded before usage — good
- add_rules_cc "MODULE.bazel"
- load("@rules_cc//cc:cc_binary.bzl", "cc_binary")
If many tests in this file need rules_cc, consider adding add_rules_cc in the existing set_up() to reduce repetition.
src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java (1)
150-160: Flip default to true — OK; fix help string spacing/grammar
- Default value for incompatible_disable_autoloads_in_main_repo is correctly flipped to "true".
- Minor nit: the help string concatenation misses a space between "in the" and "main repository", yielding "inthemain". Also consider a comma after "When enabled".
Apply this diff:
- "Controls if the autoloads (set by --incompatible_autoload_externally) are enabled in the" - + "main repository. When enabled the rules (or other symbols) that were previously " + "Controls if the autoloads (set by --incompatible_autoload_externally) are enabled in the " + + "main repository. When enabled, the rules (or other symbols) that were previously "src/test/shell/bazel/bazel_coverage_cc_test_llvm.sh (2)
32-35: Initialize rules_cc in set_up — goodDeclaring rules_cc once per test via set_up is the right approach.
Given set_up handles rules_cc, consider removing redundant add_rules_cc in setup_external_cc_target to avoid double-appends to MODULE.bazel if add_rules_cc isn't idempotent.
280-284: External repo setup: add_rules_cc and loads — mostly good
- Adding rules_cc in MODULE.bazel here is fine, but may be redundant with set_up().
- Optional future-proofing: other_repo/BUILD relies on autoloading for cc_library/cc_test. Consider adding explicit loads there too to avoid relying on autoloads outside main in future flips.
Example change to other_repo/BUILD:
+load("@rules_cc//cc:cc_library.bzl", "cc_library") +load("@rules_cc//cc:cc_test.bzl", "cc_test") cc_library( name = "a", ... ) cc_test( name = "t", ... )src/test/shell/bazel/bazel_java_test.sh (1)
91-91: Minor nit: unify quoting of MODULE.bazelSome calls use
add_rules_java MODULE.bazel(no quotes) while others use"MODULE.bazel". Consider standardizing for consistency.Also applies to: 2214-2214
src/test/shell/bazel/bazel_java_test_defaults.sh (1)
58-58: Nit: unify quoting of MODULE.bazel parameterYou mix
add_rules_java MODULE.bazelandadd_rules_java "MODULE.bazel". Consider picking one for consistency across the file.Also applies to: 136-136, 317-317, 343-343, 369-369, 426-426, 460-460
src/test/shell/bazel/bazel_rules_test.sh (1)
827-827: Minor path fix: chmod targets the wrong fileThe chmod here points to
pkg/library.shbut this section just authoredother_repo/pkg/library2.sh. For clarity, chmod the file you just created.Apply this diff:
- chmod +x pkg/library.sh + chmod +x other_repo/pkg/library2.shsrc/test/py/bazel/launcher_test.py (1)
239-254: Hard-coded rule versions – consider stanza reuseEach test amputates and rewrites a standalone
MODULE.bazel, hard-coding
rules_java/shell/pythonversions multiple times. This inflates
CI time and diverges versions across tests. Extract a small helper that
writes the module only once per language and re-use it across test
cases.src/test/shell/bazel/bazel_test_test.sh (1)
331-337: LOAD statements now duplicated
load("@rules_shell//shell:sh_test.bzl", "sh_test")is injected in many
BUILD snippets that are already underload()earlier in the same file,
producing duplicate symbols and a style warning. Remove the redundant
load()in templates that are appended multiple times.src/test/py/bazel/runfiles_test.py (2)
488-488: Heads-up: rules_python dep missing for disabled wrapped py_binaryThis test is disabled, but if you re-enable it, add a rules_python bazel_dep in MODULE.bazel (and keep the existing py_binary load) to avoid autoload failures.
545-554: Include wrapper executable in DefaultInfo.filesFor completeness (e.g., queries on files, providers), include the symlinked executable in DefaultInfo.files, not just the wrapped target’s files.
Apply this diff within the wrapper’s DefaultInfo:
- DefaultInfo( - executable = executable, - files = target[DefaultInfo].files, - data_runfiles = data_runfiles, - default_runfiles = default_runfiles, - ), + DefaultInfo( + executable = executable, + files = depset([executable], transitive = [target[DefaultInfo].files]), + data_runfiles = data_runfiles, + default_runfiles = default_runfiles, + ),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (38)
examples/py/BUILD(1 hunks)examples/py_native/BUILD(1 hunks)examples/py_native/fibonacci/BUILD(1 hunks)examples/shell/BUILD(1 hunks)src/BUILD(1 hunks)src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java(2 hunks)src/main/java/com/google/devtools/build/skyframe/BUILD(1 hunks)src/main/starlark/tests/builtins_bzl/builtin_test_setup.sh(1 hunks)src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test/BUILD.builtin_test(1 hunks)src/main/starlark/tests/builtins_bzl/cc/cc_static_library/test/BUILD.builtin_test(1 hunks)src/test/java/com/google/devtools/build/lib/blackbox/tests/PythonBlackBoxTest.java(2 hunks)src/test/py/bazel/bazel_external_repository_test.py(6 hunks)src/test/py/bazel/launcher_test.py(15 hunks)src/test/py/bazel/py_test.py(8 hunks)src/test/py/bazel/query_test.py(1 hunks)src/test/py/bazel/runfiles_test.py(7 hunks)src/test/shell/bazel/BUILD(1 hunks)src/test/shell/bazel/bazel_build_event_stream_test.sh(4 hunks)src/test/shell/bazel/bazel_coverage_cc_test_gcc.sh(10 hunks)src/test/shell/bazel/bazel_coverage_cc_test_llvm.sh(6 hunks)src/test/shell/bazel/bazel_coverage_java_test.sh(17 hunks)src/test/shell/bazel/bazel_example_test.sh(1 hunks)src/test/shell/bazel/bazel_hermetic_sandboxing_test.sh(2 hunks)src/test/shell/bazel/bazel_java_test.sh(42 hunks)src/test/shell/bazel/bazel_java_test_defaults.sh(10 hunks)src/test/shell/bazel/bazel_localtest_test.sh(2 hunks)src/test/shell/bazel/bazel_proto_library_test.sh(11 hunks)src/test/shell/bazel/bazel_random_characters_test.sh(1 hunks)src/test/shell/bazel/bazel_rules_java_override_test.sh(3 hunks)src/test/shell/bazel/bazel_rules_java_test.sh(1 hunks)src/test/shell/bazel/bazel_rules_test.sh(16 hunks)src/test/shell/bazel/bazel_sandboxing_test.sh(5 hunks)src/test/shell/bazel/bazel_symlink_test.sh(2 hunks)src/test/shell/bazel/bazel_test_test.sh(42 hunks)src/test/shell/bazel/remote/remote_build_event_uploader_test.sh(2 hunks)src/test/shell/bazel/remote/remote_execution_http_test.sh(4 hunks)src/test/shell/bazel/remote/remote_execution_test.sh(31 hunks)src/test/shell/testenv.sh(1 hunks)
🔇 Additional comments (84)
src/BUILD (1)
7-7: Confirmed rules_shell declared in MODULE.bazelRoot
MODULE.bazel(line 36) andsrc/MODULE.tools(line 44) both includebazel_dep(name = "rules_shell", version = "0.3.0")so the explicit
load("@rules_shell//shell:sh_binary.bzl", "sh_binary")is valid with autoloads disabled. No further action required.
examples/shell/BUILD (1)
1-3: Explicit shell rule loads are correct and complete.All used rules (sh_binary, sh_library, sh_test) are now explicitly loaded; this is exactly what we need with autoloads off.
examples/py_native/fibonacci/BUILD (1)
1-1: Correct explicit load for py_library.Matches the rule usage and the repo-wide style of rule-specific loads from rules_python.
src/main/starlark/tests/builtins_bzl/cc/cc_static_library/test/BUILD.builtin_test (1)
2-4: No explicitcc_testload needed here
In builtins_bzl tests, bothcc_testandcc_static_libraryare injected into the Starlark environment via the common exports (src/main/starlark/builtins_bzl/common/exports.bzl), so you don’t need to add an explicitload("@rules_cc//cc:cc_test.bzl", "cc_test")The current loads for
cc_library,cc_import, andsh_testare sufficient.Likely an incorrect or invalid review comment.
src/main/starlark/tests/builtins_bzl/builtin_test_setup.sh (1)
26-26: Nice—ensures @rules_shell is available in test MODULEs.This complements the new sh_* loads in test BUILD files and avoids missing repo errors under disabled autoloads.
examples/py/BUILD (1)
1-2: Explicitly loading py rules from rules_python looks correct.Paths and symbols match rules_python’s split .bzl files. No further changes needed.
src/test/shell/bazel/BUILD (1)
2-3: Good: sh rules are explicitly loaded from rules_shell.This aligns with the added usages of sh_library/sh_test and the dependency on @rules_shell//shell/runfiles.
examples/py_native/BUILD (1)
1-3: Correct explicit loads for py_binary/py_library/py_test.Matches the rules_python layout and the targets defined below.
src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test/BUILD.builtin_test (1)
18-21: Explicitly load cc_shared_libraryThe test at
src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test/BUILD.builtin_testinvokescc_shared_libraryrepeatedly (e.g. line 75) but only loadscc_library,cc_binary,cc_import, andsh_test. If you’ve disabled the built-in injection ofcc_shared_libraryunder your new flag, add an explicit load:• Between the existing cc_import and sh_test loads:
load("@rules_cc//cc:cc_import.bzl", "cc_import") +load("@rules_cc//cc:cc_shared_library.bzl", "cc_shared_library") load("@rules_shell//shell:sh_test.bzl", "sh_test")• Or consolidate via
defs.bzlif preferred:-load("@rules_cc//cc:cc_import.bzl", "cc_import") +load("@rules_cc//cc:defs.bzl", "cc_library", "cc_binary", "cc_import", "cc_shared_library")Please verify whether
cc_shared_libraryis still provided by the built-in injection in your current setup, and update this load accordingly.src/test/shell/bazel/bazel_example_test.sh (1)
58-60: Setup now declares rules_python and rules_shell in MODULE.bazel — good.This matches later uses of Python and Shell examples/tests in this script and reduces reliance on autoloads.
src/main/java/com/google/devtools/build/skyframe/BUILD (1)
1-1: Explicitly loadingjava_proto_libraryis correct and future-proof.This aligns with disabling autoloads and makes the dependency explicit. No issues spotted.
src/test/shell/bazel/bazel_random_characters_test.sh (1)
58-58: Ignorerules_javaload path suggestion; test scripts uniformly usejava_library.bzl
The shell tests insrc/test/shell/bazel/all load@rules_java//java:java_library.bzl(including the file under review at line 58), so switching this one todefs.bzlwould break consistency within the test suite. Leave the existing load as-is.Likely an incorrect or invalid review comment.
src/test/shell/bazel/remote/remote_build_event_uploader_test.sh (2)
431-431: Explicitly loadingsh_testis correct.This is necessary with autoloads disabled. Looks good.
459-459: Explicitsh_testload is correct here too.Consistent with the previous test case. No issues.
src/test/py/bazel/query_test.py (2)
46-51: Good: declarerules_pythonin MODULE.bazel.This makes the test independent of autoloads. No issues found.
55-55: Explicitly loadingpy_binaryis appropriate.Matches the declared
rules_pythondependency. LGTM.src/test/shell/bazel/bazel_localtest_test.sh (2)
25-25: Good: initialize shell rules in MODULE.bazel.This ensures
sh_testis available when autoloads are disabled.
41-41: Correct: loadsh_testfrom@rules_shell.Required with autoloads disabled; looks good.
src/test/shell/testenv.sh (1)
479-480: Explicit java_import load is correct and necessaryAdding
load("@rules_java//java:java_import.bzl", "java_import")ensures parsing works when autoloads are disabled. Good alignment with the PR’s objective.src/test/shell/bazel/bazel_symlink_test.sh (2)
102-102: Explicitly loading py_binary is correct
load("@rules_python//python:py_binary.bzl", "py_binary")is required with autoloads off. Looks good.
73-73: Confirmed: rules_python present in default lockThe default lock file at
src/test/tools/bzlmod/MODULE.bazel.lockincludes multiplerules_pythonentries (lines 114–121), so version resolution will succeed. No further changes required.src/test/shell/bazel/bazel_hermetic_sandboxing_test.sh (2)
66-66: Good: add rules_python to MODULE.bazelEnsures Python rules are available explicitly in the hermetic sandbox tests.
99-99: Good: explicit py_test load
load("@rules_python//python:py_test.bzl", "py_test")avoids reliance on autoloads. Consistent with the PR’s goal.src/test/shell/bazel/bazel_rules_java_test.sh (1)
71-72: Required JavaInfo load added — LGTMThis prevents an undefined symbol error at parse time and matches current rules_java layout.
src/test/java/com/google/devtools/build/lib/blackbox/tests/PythonBlackBoxTest.java (1)
56-61: Explicit py_binary load in BUILD — correct
load('@rules_python//python:py_binary.bzl', 'py_binary')is required with autoloads disabled. Good change.src/test/shell/bazel/bazel_rules_java_override_test.sh (1)
54-55: Explicit java_library load from @rules_java looks correctPath and symbol are correct for rules_java after autoloads are disabled in main repo.
src/test/shell/bazel/bazel_sandboxing_test.sh (3)
135-142: Enable Python rules explicitly — correct
- add_rules_python "MODULE.bazel" precedes BUILD creation.
- load("@rules_python//python:py_test.bzl", "py_test") matches symbol path.
184-186: Shell rules added and loaded explicitly — correct
- add_rules_shell "MODULE.bazel" added before Starlark definitions.
- load("@rules_shell//shell:sh_test.bzl", "sh_test") loads the right rule.
Also applies to: 223-224
716-722: Shell rules for hermetic tmp testExplicit sh_binary load is correct and aligns with flipped autoloads.
src/test/shell/bazel/bazel_build_event_stream_test.sh (4)
188-196: sh_binary test: explicit module and load — good
- add_rules_shell "MODULE.bazel" is added before BUILD.
- load("@rules_shell//shell:sh_binary.bzl", "sh_binary") matches symbol path.
Also applies to: 191-196
226-234: Second sh_binary test mirrors the first — goodSame pattern; consistent and correct.
Also applies to: 229-234
268-276: sh_test test: explicit module and load — good
- add_rules_shell "MODULE.bazel" first.
- load("@rules_shell//shell:sh_test.bzl", "sh_test") is correct.
Also applies to: 271-276
306-314: Second sh_test case — consistentPattern and ordering are correct.
Also applies to: 309-314
src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java (1)
953-954: Semantics key prefix updated to match flipped default — goodChanging to "+incompatible_disable_autoloads_in_main_repo" is consistent with the new default.
Ensure any internal docs and migration notes referencing the default are updated accordingly (e.g., site/docs/skylark/backward-compatibility.md).
src/test/shell/bazel/bazel_coverage_cc_test_llvm.sh (4)
74-77: Add explicit cc_library/cc_test loads — correctThe load paths match rules_cc’s structure. Good ordering prior to rule usage.
211-217: Java target uses explicit rules — correct
- add_rules_java "MODULE.bazel"
- load("@rules_java//java:java_binary.bzl", "java_binary")
- cc loads remain explicit.
442-444: Loads for //foo/tmp BUILD — correctExplicit rules_cc loads are present before usage.
515-517: Loads for header coverage BUILD — correctExplicit cc_library and cc_test loads are correctly added.
src/test/shell/bazel/remote/remote_execution_http_test.sh (1)
76-76: Explicit rule deps and loads for cc and shell — LGTMMaking loads explicit and adding
rules_cc/rules_shellvia MODULE.bazel aligns the tests with disabled autoloads.Also applies to: 79-79, 111-111, 114-114, 147-147, 178-178
src/test/shell/bazel/bazel_java_test.sh (1)
91-92: Broad consistency pass: explicit loads and module deps — LGTMAcross these hunks you consistently:
- add
add_rules_java/add_rules_shell/add_rules_ccto MODULE.bazel before usage, and- load rules from
@rules_java,@rules_shell, and@rules_ccin BUILD snippets.This is the right approach for disabling autoloads. No issues spotted.
Also applies to: 97-97, 118-118, 149-150, 388-389, 433-434, 682-684, 709-710, 825-827, 889-892, 945-947, 1599-1600, 1632-1640, 1689-1696, 1730-1731, 1794-1796, 1904-1907, 1991-1994, 2091-2092, 2127-2128, 2145-2147, 2175-2176, 2214-2214, 2237-2238, 2261-2267, 2311-2316, 2335-2336, 2352-2353, 2414-2415, 2445-2446
src/test/py/bazel/py_test.py (1)
29-33: Explicit Python rule loads + MODULE.bazel setup — LGTM
- BUILD fragments correctly load
py_binary,py_library, andpy_testfrom@rules_python.setUpaddsrules_pythonto MODULE.bazel before use.
No functional changes beyond rule loading. Looks good.Also applies to: 50-54, 79-84, 87-96, 139-149, 176-181, 189-197, 211-225, 256-287, 310-312
src/test/shell/bazel/bazel_java_test_defaults.sh (1)
58-63: Java rule loads and module deps made explicit — LGTMThese hunks correctly:
- declare
rules_java/rules_ccin MODULE.bazel before usage, and- load
java_*andcc_*rules explicitly in BUILD snippets.Matches the goal of disabling autoloads.
Also applies to: 98-102, 135-140, 257-265, 318-321, 344-347, 370-373, 400-400, 429-430, 463-464
src/test/shell/bazel/bazel_rules_test.sh (2)
63-67: Shell/Java/C++ rule explicitness — LGTMThe tests now:
- add
rules_shell/rules_java/rules_ccto MODULE.bazel before use, and- load
sh_*,java_*, andcc_*rules explicitly in BUILD files.This is consistent with autoload disabling. No functional issues spotted.
Also applies to: 101-103, 150-152, 189-191, 217-220, 235-235, 241-243, 344-349, 682-685, 779-782, 1039-1044, 1089-1090, 1144-1145, 1161-1162, 1219-1220
63-64: Correct and rerun the duplicate-deps checkThe previous awk pattern was malformed (unescaped parentheses), so it never matched anything. After running your bazel-rules tests locally (to regenerate MODULE.bazel), please execute:
#!/bin/bash # After running tests, check for duplicate deps in MODULE.bazel grep -E '^bazel_dep\(name = "rules_(shell|java|cc|python)"\)' MODULE.bazel | sort | uniq -cIf any line is reported with a count > 1, update the corresponding add_rules_* helper to guard against inserting the same rule twice.
src/test/shell/bazel/bazel_coverage_java_test.sh (18)
45-47: Good: centralized module setup for Java rulesAdding set_up() with add_rules_java ensures MODULE.bazel is prepared before BUILD generation across tests.
52-53: Explicit java_test/java_library loadsBUILD content now correctly loads rules from @rules_java before usage.
148-150: LGTM: explicit loads in combined report testExplicitly loading java_library and java_test aligns with autoloads disabled policy.
240-243: LGTM: explicit loads for java_import scenariojava_test/java_import/java_library are loaded from @rules_java as required.
335-336: LGTM: load java_binary in producer BUILDEnsures java_binary is available from rules_java for Cov jar generation.
359-359: LGTM: load java_test in consumer BUILDExplicit java_test load is correct.
428-430: LGTM: explicit loads for java_binary/java_libraryRuntime-deps test now correctly sources rules from @rules_java.
469-469: LGTM: explicit load for java_testConsistent with the rest of the file.
502-502: LGTM: explicit load for java_binaryRequired for deploy jar tests.
543-543: LGTM: explicit load for java_testMatches the rules_java migration pattern.
650-652: LGTM: explicit loads for classpath-jar coveragejava_library/java_test loads from @rules_java are correct.
717-720: Verify rules_java attribute compatibility (use_launcher, deploy_manifest_lines)This BUILD uses java_binary attributes commonly provided by native rules: deploy_manifest_lines and use_launcher. Confirm that the pinned rules_java version in add_rules_java supports these attributes with the same semantics.
If needed, run the affected test in isolation to catch attribute mismatches early.
819-821: LGTM: explicit loads in string-switch coverage testjava_test/java_library loads are correct.
914-916: LGTM: explicit loads in finally-block coverage testConsistent with explicit-loading policy.
1122-1125: Cross-language coverage setup looks good; verify java_test's use_testrunner
- add_rules_cc prepares MODULE.bazel; explicit java_test load added.
- test uses java_test(use_testrunner = False). Ensure rules_java's java_test supports use_testrunner.
Consider running just this test to ensure rules_java’s java_test accepts use_testrunner and that the expected deploy/run artifacts exist.
1147-1149: LGTM: explicit loads for cc_binary/cc_libraryCorrectly loads from @rules_cc in the example C++ BUILD.
1274-1275: LGTM: explicit load for java_library in main repo targetKeeps main repo BUILD compatible with disabled autoloads.
1299-1301: LGTM: explicit loads in external repo BUILDEnsures @other_repo can resolve java_library/java_test from rules_java.
src/test/py/bazel/bazel_external_repository_test.py (7)
69-77: LGTM: MODULE.bazel declares rules_python and defines http_archive repo ruleExplicit rules_python dependency and clearer use_repo_rule formatting are correct.
82-82: LGTM: explicit load of py_libraryBUILD file properly loads py_library from @rules_python.
189-199: LGTM: MODULE.bazel declares rules_python and http_archive for sparse tar testExplicit module dependency and repo rule definition look correct.
220-229: LGTM: explicit load of py_test and use of rlocationpath
- py_test is correctly loaded from @rules_python.
- args uses $(rlocationpath …) and data includes the referenced target.
255-260: LGTM: MODULE.bazel declares rules_python and new_local_repository use_repo_ruleConsistent with disabling autoloads; formatting is clearer and explicit.
269-277: LGTM: BUILD now loads py_binary and custom Starlark ruleExplicit loading ensures @r//:foo.bzl and rules_python are available.
294-297: LGTM: local_repository use_repo_rule formattingConsistent and explicit definition in MODULE.bazel for my_repo setup.
src/test/py/bazel/runfiles_test.py (9)
266-274: LGTM: MODULE.bazel declares rules_python and local_repository use_repo_ruleSets up module deps explicitly; correct ordering and syntax.
276-293: LGTM: py_binary explicitly loaded and configured
- py_binary loaded from @rules_python.
- deps include @bazel_tools//tools/python/runfiles to make import available.
309-311: LGTM: MODULE.bazel declares rules_shell for sh_testPrepares the module for shell rules usage.
315-315: LGTM: explicit load of sh_testKeeps BUILD compatible with disabled autoloads.
435-436: LGTM: MODULE.bazel declares rules_shell for no-runfiles testMatches usage of sh_test below.
440-440: LGTM: explicit load of sh_testConsistent with the overall change.
456-457: LGTM: MODULE.bazel declares rules_shell for wrapped sh_binaryRequired for sh_binary load below.
461-461: LGTM: explicit load of sh_binaryCorrect rule source for shell binaries.
509-515: LGTM: MODULE.bazel declares rules_java and BUILD loads java_binaryExplicit Java rule usage aligns with the PR objective.
src/test/shell/bazel/remote/remote_execution_test.sh (6)
310-310: Explicitly declaring rules_cc module usage looks correct.add_rules_cc "MODULE.bazel" is consistently added before using cc_* rules. This aligns with disabling autoloads and should prevent implicit rule resolution.
Also applies to: 339-339, 363-363, 387-387, 415-415, 441-441, 631-631, 2449-2449, 2588-2588, 3072-3072, 3240-3240
313-313: rules_cc loads are correct and complete.All BUILD fragments now explicitly load the needed Bazel rules from @rules_cc (cc_binary, cc_test, cc_library, cc_import). No missing or incorrect symbols observed.
Also applies to: 342-342, 366-366, 390-390, 418-418, 444-444, 635-635, 2454-2454, 2456-2456, 2593-2593, 2595-2595, 2596-2596, 3080-3080, 3100-3100, 3248-3248, 3250-3250, 3277-3277
688-688: Python rule wiring is sound.add_rules_python "MODULE.bazel" precedes usage and the BUILD files load py_test from @rules_python. This should work with autoloads disabled.
Also applies to: 712-712, 752-752, 692-692, 716-716, 756-756
817-817: Shell rules are explicitly declared and loaded.Consistent use of add_rules_shell "MODULE.bazel" and corresponding load()s for sh_test/sh_binary from @rules_shell. Covers all affected tests.
Also applies to: 821-821, 853-853, 857-857, 1251-1251, 1260-1260, 2075-2075, 2082-2082, 2728-2728, 2731-2731, 2813-2813, 2816-2816, 3336-3336, 3338-3338, 3364-3364, 3366-3366, 3619-3619, 3660-3660
2158-2158: Java rules: explicit deps and loads look correct.add_rules_java "MODULE.bazel" added and both java_library and java_test are properly loaded from @rules_java in the BUILD file.
Also applies to: 2167-2169
3248-3250: External repo scenarios also correctly load rules_cc.Loads for cc_binary/cc_import within other_repo ensure external BUILD files don’t rely on autoloads. Good coverage for cross-repo cases.
Also applies to: 3277-3277
|
/refacto-test |
|
PR already reviewed at the latest commit: 25c484c. |
|
/refacto-test |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
Code Review: Autoloads Configuration and Module System👍 Well Done
📌 Files Processed
📝 Additional Comments
|
![refacto-test[bot]](https://avatars.githubusercontent.com/u/196302493?s=60&v=4){kind=small}
| @Option( | ||
| name = "incompatible_disable_autoloads_in_main_repo", | ||
| defaultValue = "false", | ||
| defaultValue = "true", |
There was a problem hiding this comment.
Default Flag Mismatch
The incompatible_disable_autoloads_in_main_repo flag's default value is changed from false to true, but the flag name still has the 'incompatible_' prefix. This creates inconsistent behavior as incompatible flags are typically temporary and meant to be removed after migration.
@Option(
name = "disable_autoloads_in_main_repo",
defaultValue = "true",
Commitable Suggestion
| defaultValue = "true", | |
| @Option( | |
| name = "disable_autoloads_in_main_repo", | |
| defaultValue = "true", |
Standards
- ISO-IEC-25010-Reliability-Maturity
- ISO-IEC-25010-Functional-Correctness-Appropriateness
| "-incompatible_stop_exporting_language_modules"; | ||
| public static final String INCOMPATIBLE_DISABLE_AUTOLOADS_IN_MAIN_REPO = | ||
| "-incompatible_disable_autoloads_in_main_repo"; | ||
| "+incompatible_disable_autoloads_in_main_repo"; |
There was a problem hiding this comment.
Flag Reference Inconsistency
The flag reference is changed from negative to positive prefix, but the flag name itself still contains 'incompatible_'. This creates inconsistent behavior between the flag's default value and its reference pattern, potentially causing configuration errors.
Commitable Suggestion
| "+incompatible_disable_autoloads_in_main_repo"; | |
| public static final String DISABLE_AUTOLOADS_IN_MAIN_REPO = | |
| "-disable_autoloads_in_main_repo"; |
Standards
- ISO-IEC-25010-Reliability-Maturity
- ISO-IEC-25010-Functional-Correctness-Appropriateness
| "-incompatible_stop_exporting_language_modules"; | ||
| public static final String INCOMPATIBLE_DISABLE_AUTOLOADS_IN_MAIN_REPO = | ||
| "-incompatible_disable_autoloads_in_main_repo"; | ||
| "+incompatible_disable_autoloads_in_main_repo"; |
There was a problem hiding this comment.
Flag Name Inconsistency
The flag name prefix is changed from minus to plus, indicating a change in the default behavior. This should be documented with a comment explaining the semantic change and its impact on build configurations.
Standards
- Clean-Code-Comments
- Maintainability-Quality-Documentation
|
/refacto-test |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
3 participants
The flag flip forces loads in main repository when using Bazel@HEAD. Flipping it now, so that we can disable autoloads by release of Bazel 9.
PiperOrigin-RevId: 745526309
Change-Id: I7204470c08a66a15b28bc0b0c48f41d69e1694c8
Summary by CodeRabbit